Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail-safe handling of nullable string value in lookupStyle #1031

Merged
merged 1 commit into from
Mar 7, 2019

Conversation

flugg
Copy link
Contributor

@flugg flugg commented Mar 7, 2019

Hi, we've been doing some research to find out why the flex layout library throws errors on older browsers. More specifically, Safari on iOS 8 and below.

This issue reports about this error: #958 - it says it's a duplicate of #947, however, these errors seem slightly unrelated.

The problem is the lookupStyle method in the StyleUtils service calls getComputedStyle(element).getPropertyValue(styleName). The spec says that getPropertyValue should return an empty string if the style is not set, however, on some older browsers it returns null instead. This causes an error when the method returns value.trim(), since value isn't a string.

This PR adds a safeguard to check for null and makes sure the method always return a string.

We've tested this on multiple old browsers on Browserstack and this seems to solve all our issues.

Make sure the lookupStyle method in StyleUtils return a string. On old
browsers getPropertyValue() of getComputedStyle() returns a null instead
of empty string.

Fixes angular#958
Copy link
Member

@CaerusKaru CaerusKaru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@CaerusKaru CaerusKaru self-assigned this Mar 7, 2019
@CaerusKaru CaerusKaru added pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for the caretaker to presubmit and merge labels Mar 7, 2019
@CaerusKaru CaerusKaru added this to the 7.0.0-beta.24 milestone Mar 7, 2019
@CaerusKaru CaerusKaru merged commit 5112a47 into angular:master Mar 7, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for the caretaker to presubmit and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants